shepherd.js/package.json (1)

21-34: CJS + ESM dual export setup looks correct.

The conditional exports are properly structured, and .cjs extension ensures CommonJS semantics regardless of the "type": "module" field. Good that @arethetypeswrong/cli is in devDependencies and the types:check:attw script validates the package exports.

One minor note: there's no top-level "types" field alongside "main". Tools that don't understand the "exports" map (e.g., TypeScript < 4.7 with older moduleResolution settings) won't resolve type declarations for the CJS entry. If you want to support those consumers, consider adding:

"types": "./dist/cjs/shepherd.d.cts",

Not critical given modern tooling and the breaking-change nature of this PR.

shepherd.js/src/utils/dom.ts (1)

40-44: Consider using a more explicit event listener prefix like on- or on:.

The on prefix check (key.startsWith('on')) could inadvertently match non-event attributes (e.g., hypothetical oneTimeUse). The AI summary mentions on- as the convention, but the implementation uses on (camelCase style like onClick). This works fine for the current internal usage but is worth noting if this utility is extended.

shepherd.js/src/components/shepherd-title.ts (1)

14-15: Consider textContent instead of innerHTML for the title.

Since StringOrStringFunction resolves to a plain string, textContent would be safer and sufficient here unless HTML-in-titles is an intentional feature. If HTML support is needed, a brief doc comment clarifying that would help.

Proposed change

   const resolvedTitle = isFunction(title) ? title() : title;
-  el.innerHTML = resolvedTitle;
+  el.textContent = resolvedTitle;

shepherd.js/src/components/shepherd-button.ts (2)

34-36: innerHTML with user-supplied button text — XSS surface by design.

The StepOptionsButton.text JSDoc says "The HTML text of the button," so this is intentional. However, since text can come from a dynamic function (getConfigOption), any user who passes unsanitized input through the config is exposed. Consider documenting this trust boundary or offering a textContent path for plain-text labels.

---

23-32: Class string can contain leading/trailing whitespace and double spaces.

When config.classes is undefined, the template literal produces a leading space (e.g., " shepherd-button "). Similarly, when secondary is false, there's a trailing space. While not functionally broken, classList and selector matching can be subtly affected by stray whitespace in some environments.

♻️ Suggested cleanup

-  const btn = h('button', {
-    'aria-label': label || null,
-    class: `${config.classes || ''} shepherd-button ${
-      config.secondary ? 'shepherd-button-secondary' : ''
-    }`,
+  const classes = [
+    config.classes,
+    'shepherd-button',
+    config.secondary ? 'shepherd-button-secondary' : null
+  ].filter(Boolean).join(' ');
+
+  const btn = h('button', {
+    'aria-label': label || null,
+    class: classes,

shepherd.js/test/unit/components/shepherd-content.spec.js (1)

1-82: Tests are correct, but coverage is limited to header rendering.

The header conditional logic is well-tested across all four scenarios. However, the test suite only covers the header portion of createShepherdContent. There are no tests for text rendering (step.options.text) or footer rendering (step.options.buttons).

shepherd.js/src/step.ts (1)

686-688: Duplicate this.el.hidden = false assignment.

Lines 673-674 and 686-688 both set this.el.hidden = false. This appears to be a pre-existing duplication (both are unchanged lines), but since this file is being reworked it's a good time to clean it up.

shepherd.js/test/unit/components/shepherd-modal.spec.js (1)

432-441: Skipped setupForStep tests reduce coverage.

The comment explains the spy limitation, but setupForStep is a key integration point (it wires up per-step modal behavior). Consider testing the observable outcome instead — e.g., after calling setupForStep(step) with a step configured for modal overlay, assert that the modal element has the shepherd-modal-is-visible class and that the path d attribute reflects the target element's position.

shepherd.js/test/unit/components/shepherd-button.spec.js (1)

18-18: Passing undefined as step is fragile and masks potential NPEs.

Every call uses createShepherdButton({...}, undefined), but the factory signature expects a Step instance (e.g., config.action.bind(step.tour) would throw if action were set). Consider creating a minimal mock/stub Step to avoid silent breakage if the factory internals ever touch step for the options being tested (e.g., getConfigOption receives step).

shepherd.js/test/unit/components/shepherd-element.spec.js (1)

6-8: keyCode is deprecated in favor of key or code.

Both the helper and the production code (shepherd-element.ts lines 50, 93, 100, 107) use the deprecated KeyboardEvent.keyCode. This is fine for now since it matches, but worth noting as a follow-up to migrate both to event.key (e.g., 'Escape', 'ArrowLeft', 'ArrowRight', 'Tab').

shepherd.js/src/components/shepherd-element.ts (1)

48-117: keyCode is deprecated — consider migrating to event.key.

KeyboardEvent.keyCode is deprecated in modern browsers. Using event.key ('Tab', 'Escape', 'ArrowLeft', 'ArrowRight') would be more future-proof and readable. Not urgent for this PR since it's a straight port from the Svelte version.

shepherd.js/src/components/shepherd-modal.ts (1)

46-294: No public destroy/dispose method to remove the SVG element and the touchmove listener.

The factory appends the SVG to container (line 66) and registers a touchmove listener on it (line 61), but the returned API has no way to tear these down. If the modal is recreated (e.g., on re-init), the old SVG and listener would leak. Consider adding a destroy() method that removes the element and its listeners.

shepherd.js/src/utils/dom.ts (1)

37-47: Consider handling value === true for boolean HTML attributes.

When value is true and the key is not 'disabled' (e.g., 'hidden', 'required', 'autofocus'), setAttribute(key, 'true') is called, which sets the attribute to the string "true". For standard boolean HTML attributes, the correct representation is the empty string or the attribute name itself (e.g., setAttribute('hidden', '')). In practice this still works in browsers (any value makes a boolean attribute truthy), but 'open' on line 128 of shepherd-element.ts is set to the string 'true' via this path, producing <dialog open="true"> instead of the more canonical <dialog open="">.

This is a very minor correctness nit — browsers handle it fine.

🔧 Optional: canonical boolean attribute handling

     } else if (key === 'disabled' && value === true) {
       (el as HTMLButtonElement).disabled = true;
+    } else if (value === true) {
+      el.setAttribute(key, '');
     } else {
       el.setAttribute(key, String(value));
     }

shepherd.js/src/components/shepherd-element.ts (2)

6-9: e.keyCode is deprecated — prefer e.key string comparisons.

KeyboardEvent.keyCode is deprecated in the DOM spec. Using e.key is the modern, more readable approach and avoids magic numbers.

♻️ Proposed refactor

-const KEY_TAB = 9;
-const KEY_ESC = 27;
-const LEFT_ARROW = 37;
-const RIGHT_ARROW = 39;

Then in handleKeyDown:

-  const handleKeyDown = (e: KeyboardEvent) => {
-    const { tour } = step;
-    switch (e.keyCode) {
-      case KEY_TAB:
+  const handleKeyDown = (e: KeyboardEvent) => {
+    const { tour } = step;
+    switch (e.key) {
+      case 'Tab':
         ...
-      case KEY_ESC:
+      case 'Escape':
         ...
-      case LEFT_ARROW:
+      case 'ArrowLeft':
         ...
-      case RIGHT_ARROW:
+      case 'ArrowRight':
         ...

Also applies to: 48-113

---

23-41: Type of hasTitle is string | false, not boolean.

step.options?.title ?? false yields the title string (or the result of calling it if it were already resolved) when truthy, or false when nullish. This works correctly in the conditional on line 122 (hasTitle ? ... : ''), but the inferred type is string | false rather than a clean boolean. Consider using !! for clarity:

-  const hasTitle = step.options?.title ?? false;
+  const hasTitle = !!step.options?.title;

This is a minor readability nit — behavior is identical.

shepherd.js/test/unit/components/shepherd-modal.spec.js (1)

433-440: Skipped setupForStep tests leave a coverage gap that's straightforward to fill.

Instead of spying on hide/show, you can assert the observable outcome directly: check whether modal.getElement() has the shepherd-modal-is-visible class after calling setupForStep with a mock step. For example:

it('useModalOverlay: false, hides modal', () => {
  const modal = createShepherdModal(container);
  modal.show(); // start visible
  modal.setupForStep({ options: {}, tour: { options: { useModalOverlay: false } } });
  expect(modal.getElement()).not.toHaveClass('shepherd-modal-is-visible');
});

You'd also need to call modal.hide() in afterEach (or add a cleanup/destroy to the API) to cancel the rAF loop started by _styleForStep when useModalOverlay: true.

shepherd.js/src/components/shepherd-modal.ts (1)

200-215: Minor: setting rafId = undefined before scheduling the next frame creates a brief window where cleanup can't cancel the loop.

In rafLoop, rafId is set to undefined at the top (Line 201), then a new frame is requested at the bottom (Line 211). If _cleanupStepEventListeners is called during the synchronous execution of positionModal (e.g., via a step callback that triggers cleanup), the cancelAnimationFrame check at Line 179 would see rafId as undefined and miss cancelling the already-queued next frame.

A safer pattern is to only clear rafId when explicitly cancelled:

Proposed fix

     const rafLoop = () => {
-      rafId = undefined;
       positionModal(
         modalOverlayOpeningPadding,
         modalOverlayOpeningRadius,
         modalOverlayOpeningXOffset + iframeOffset.left,
         modalOverlayOpeningYOffset + iframeOffset.top,
         scrollParent,
         step.target,
         step._resolvedExtraHighlightElements
       );
       rafId = requestAnimationFrame(rafLoop);
     };
 
-    rafLoop();
+    rafId = requestAnimationFrame(rafLoop);
     _addStepEventListeners();

shepherd.js/rollup.config.mjs (2)

162-163: CJS build re-runs PostCSS extraction unnecessarily.

The shared plugins array includes the postcss({ extract: 'css/shepherd.css' }) plugin, so the CJS build will also extract CSS into tmp/cjs/css/shepherd.css — which is never used. Consider either excluding PostCSS from the CJS plugins or accepting the minor build-time cost.

Possible fix

     plugins: [
-      ...plugins,
+      ...plugins.filter((p) => p.name !== 'postcss'),
       {
         name: 'After CJS build tweaks',

---

114-119: Consider invoking tsc directly instead of via npx.

Since typescript is already a devDependency and available in node_modules/.bin, using npx tsc adds unnecessary lookup overhead on every build. A direct tsc (or pnpm tsc) call would be marginally faster and more predictable.

shepherd.js/src/components/shepherd-modal.ts (1)

207-218: rafId is cleared before positionModal runs — reentrant cleanup can't cancel this frame.

Setting rafId = undefined (line 208) before positionModal (line 209) means if any code path during positionModal calls _cleanupStepEventListeners, the cancelAnimationFrame is a no-op, and line 218 schedules a new frame anyway. Moving the assignment after requestAnimationFrame avoids this:

Suggested tweak

     const rafLoop = () => {
-      rafId = undefined;
       positionModal(
         modalOverlayOpeningPadding,
         modalOverlayOpeningRadius,
         modalOverlayOpeningXOffset + iframeOffset.left,
         modalOverlayOpeningYOffset + iframeOffset.top,
         scrollParent,
         step.target,
         step._resolvedExtraHighlightElements
       );
       rafId = requestAnimationFrame(rafLoop);
     };
+
+    rafId = requestAnimationFrame(rafLoop);
-
-    rafLoop();

This also avoids the first positionModal call being synchronous (outside RAF), which is a minor behavioral difference but aligns the first frame with subsequent ones.

shepherd.js/src/components/shepherd-modal.ts (2)

213-228: rafId is briefly undefined inside rafLoop, creating a small cancellation gap.

At line 214 rafId is set to undefined before the synchronous positionModal call. If positionModal (or anything it triggers synchronously) calls _cleanupStepEventListeners, the subsequent requestAnimationFrame at line 224 won't be tracked or cancelled. Consider deferring the reset or restructuring:

Suggested approach

     const rafLoop = () => {
-      rafId = undefined;
       positionModal(
         modalOverlayOpeningPadding,
         modalOverlayOpeningRadius,
         modalOverlayOpeningXOffset + iframeOffset.left,
         modalOverlayOpeningYOffset + iframeOffset.top,
         scrollParent,
         step.target,
         step._resolvedExtraHighlightElements
       );
       rafId = requestAnimationFrame(rafLoop);
     };

Or guard the re-schedule with a disposed flag.

---

231-243: _getScrollParent may fail for elements inside iframes due to cross-realm instanceof HTMLElement.

When el originates from an iframe's document, el instanceof HTMLElement checks against the parent window's HTMLElement constructor, which returns false. This would cause overflowY to always be false, and the function would recurse up without ever finding a scrollable parent inside the iframe.

This is a known cross-realm limitation and may not be an issue if targets are always in the same document, but worth noting given the iframe offset support.

shepherd.js/test/unit/components/shepherd-modal.spec.js (2)

14-16: Consider calling modal.destroy() in afterEach to prevent window-level listener leaks.

Tests that invoke setupForStep with useModalOverlay: true (e.g., the _getScrollParent and _getIframeOffset tests) add a touchmove listener on window via _addStepEventListeners, but only container.remove() is called in teardown. The window listener survives, potentially polluting subsequent tests.

Suggested pattern

+  let modal;
+
   beforeEach(() => {
     container = document.createElement('div');
     document.body.appendChild(container);
   });

   afterEach(() => {
+    if (modal) {
+      modal.destroy();
+      modal = null;
+    }
     container.remove();
   });

Then assign modal = createShepherdModal(container) in each test (or restructure per describe block).

---

654-722: Modifying document.defaultView is fragile — it affects all elements sharing the document.

The test overrides targetEl.ownerDocument.defaultView, but in jsdom all elements share one document, so this temporarily breaks the global document.defaultView for everything. The careful save/restore at lines 704–716 mitigates this, but any assertion failure before the restore would leave the document in a broken state for all subsequent tests.

Consider wrapping the assertion + restore in a try/finally block:

Suggestion

       modal.setupForStep(step);

-      // Restore defaultView before any assertions (jsdom needs it for instanceof checks)
-      if (origDescriptor) {
-        Object.defineProperty(
-          targetEl.ownerDocument,
-          'defaultView',
-          origDescriptor
-        );
-      } else {
-        Object.defineProperty(targetEl.ownerDocument, 'defaultView', {
-          value: window,
-          configurable: true
-        });
-      }
-
-      expect(modal.getElement()).toHaveClass('shepherd-modal-is-visible');
+      try {
+        expect(modal.getElement()).toHaveClass('shepherd-modal-is-visible');
+      } finally {
+        if (origDescriptor) {
+          Object.defineProperty(
+            targetEl.ownerDocument,
+            'defaultView',
+            origDescriptor
+          );
+        } else {
+          Object.defineProperty(targetEl.ownerDocument, 'defaultView', {
+            value: window,
+            configurable: true
+          });
+        }
+      }

shepherd.js/src/components/shepherd-modal.ts (1)

210-226: iframeOffset is captured once but used across rAF frames.

_getIframeOffset is called once at line 211, then the result is reused in every rafLoop iteration. If the page layout shifts (e.g., responsive resize, lazy-loaded content pushing the iframe), the offset will be stale until the next setupForStep call. Consider moving it inside rafLoop if accuracy during layout shifts matters — the cost is negligible.

shepherd.js/src/components/shepherd-header.ts (1)

7-18: Clean composition pattern — header factory looks good.

The conditional rendering of title and cancel icon is straightforward and correct.

Nit on line 14: optional chaining would be slightly more concise:

Optional chaining

-  if (step.options.cancelIcon && step.options.cancelIcon.enabled) {
+  if (step.options.cancelIcon?.enabled) {

shepherd.js/src/components/shepherd-button.css (1)

12-15: Redundant color declaration in hover state.

Line 14's color: rgba(255, 255, 255, 0.75) is identical to the base .shepherd-button color on line 5. It can be removed unless it's there intentionally to prevent inheritance issues.

shepherd.js/src/components/shepherd-element.css (1)

37-49: Minor inconsistency: mixed pseudo-element selector syntax.

Line 38 uses ::before (modern double-colon) while line 45 uses :before (legacy single-colon). Both work in all browsers, but it's slightly inconsistent. Consider standardizing on ::before throughout.

Consistency fix

-.shepherd-arrow:before {
+.shepherd-arrow::before {
   content: '';

shepherd.js/src/components/shepherd-button.ts (2)

24-33: Class string may contain extra whitespace.

When config.classes is undefined, the template literal produces " shepherd-button ..." with a leading space. Similarly, when secondary is false, a trailing space remains. While browsers tolerate extra whitespace in the class attribute, it produces a messier DOM.

Cleaner class construction using filter/join

-  const btn = h('button', {
-    'aria-label': label || null,
-    class: `${config.classes || ''} shepherd-button ${
-      config.secondary ? 'shepherd-button-secondary' : ''
-    }`,
+  const classes = [
+    config.classes,
+    'shepherd-button',
+    config.secondary ? 'shepherd-button-secondary' : null
+  ]
+    .filter(Boolean)
+    .join(' ');
+
+  const btn = h('button', {
+    'aria-label': label || null,
+    class: classes,

---

6-11: getConfigOption return type is unknown — callers use unsafe casts.

The function returns unknown, forcing callers to use as string casts (e.g., line 36). Consider using a generic to preserve type safety:

Generic version

-function getConfigOption(option: unknown, step: Step): unknown {
+function getConfigOption<T>(option: T | ((...args: unknown[]) => T), step: Step): T {
   if (isFunction(option)) {
-    return option.call(step);
+    return option.call(step) as T;
   }
-  return option;
+  return option as T;
 }

shepherd.js/src/components/shepherd-element.ts (3)

155-177: Focusable-element query runs before the dialog is in the DOM — currently correct but fragile.

The querySelectorAll on Lines 159–162 works because children are already appended to element. However, the list is never refreshed, so dynamically added focusable elements (e.g., via updateStepOptions) won't participate in tab trapping. If this mirrors the prior Svelte behavior, it's acceptable for now, but worth a // TODO noting the limitation.

Also, on Line 168, attachToElement.tabIndex = 0 unconditionally makes the target tabbable. The original is stored, but if the element already had a meaningful tabIndex (e.g., -1 for programmatic-only focus), overriding it to 0 changes the page's tab order outside the tour. This is likely carried over from the Svelte implementation, so flagging for awareness only.

---

24-27: cleanup() does not null out closure references — minor memory-leak surface.

After cleanup() is called, the closure still holds references to step, attachToElement, and the DOM arrays. In the normal flow _teardownElements also drops this.el and the component reference, so GC should collect the whole graph. No action required unless long-lived step instances are reused without full teardown.

Also applies to: 179-183

---

49-114: Replace deprecated e.keyCode with e.key string comparisons.

KeyboardEvent.keyCode has been deprecated and is not recommended for modern development. Using e.key is more readable and future-proof.

Proposed refactor

-const KEY_TAB = 9;
-const KEY_ESC = 27;
-const LEFT_ARROW = 37;
-const RIGHT_ARROW = 39;
+const KEY_TAB = 'Tab';
+const KEY_ESC = 'Escape';
+const LEFT_ARROW = 'ArrowLeft';
+const RIGHT_ARROW = 'ArrowRight';

Then replace e.keyCode with e.key:

-    switch (e.keyCode) {
+    switch (e.key) {